Skip to content

Apply quality-of-life fixes#3

Merged
sangwa merged 10 commits intomainfrom
qol-fixes
Mar 26, 2026
Merged

Apply quality-of-life fixes#3
sangwa merged 10 commits intomainfrom
qol-fixes

Conversation

@sangwa
Copy link
Copy Markdown
Contributor

@sangwa sangwa commented Mar 26, 2026

  • refactor: use dustin/go-humanize for http.cache.size parsing
  • fix: default bind address to 127.0.0.1 instead of 0.0.0.0
  • feat: make HTTP cache default TTL configurable (http.cache.default_ttl)
  • fix: fail on invalid duration and byte-size config values

Summary by CodeRabbit

  • Configuration

    • Default server bind address changed from 0.0.0.0 to 127.0.0.1.
    • New http.cache.default_ttl setting added (default 1h) to control fallback cache TTL.
  • Improvements

    • Revocation collateral and endorsement caching now use the configurable default TTL and enforce a maximum cap.
    • Duration and byte-size parsing strengthened (broader format support, stricter validation/overflow checks).
  • Tests

    • Expanded TTL and byte-size parsing tests and fuzz corpus.
  • Chores

    • Updated Go module dependency declarations.

sangwa and others added 4 commits March 25, 2026 18:02
Replace hand-rolled parseByteSize with humanize.ParseBytes, adding
support for SI units (KB, MB, GB), fractional values, and flexible
whitespace.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The server runs behind an Envoy reverse proxy, so there is no need to
bind on all interfaces by default.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add http.cache.default_ttl config option (env:
ATTESTATION_SERVER_HTTP_CACHE_DEFAULT_TTL) to control the fallback
TTL when responses have no Cache-Control header. Default bumped from
30m to 1h.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, parse errors for endorsements.client.timeout,
http.cache.size, http.cache.default_ttl, revocation.refresh_interval,
and ratelimit.stall_timeout were silently replaced with defaults. This
masked misconfigurations in a security-critical service. Now the server
exits with a clear error on any unparseable value. Viper defaults still
apply when no override is provided.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sangwa sangwa self-assigned this Mar 26, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 26, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR narrows the server default bind address from 0.0.0.0 to 127.0.0.1, adds http.cache.default_ttl = "1h" with an environment binding, and exposes it as Config.HTTPCacheDefaultTTL. Duration and byte-size parsing moved to helpers that return parse errors (no silent fallbacks); byte-size parsing now uses go-humanize with overflow checks. Cache TTL logic for external fetches (cosign, endorsements, TDX revocation) now uses the configured default TTL (capped) when headers are absent. Two modules were promoted from indirect to direct requires in go.mod. Tests were updated and expanded.

Sequence Diagram(s)

sequenceDiagram
  participant Client as Client
  participant Server as Server
  participant Fetcher as Fetcher (cachedHTTPSGetter)
  participant External as External API
  participant Cache as HTTP Cache

  Client->>Server: Request resource
  Server->>Fetcher: Request external data
  Fetcher->>Cache: Check cache for item
  alt Cache hit
    Cache-->>Fetcher: Return cached response + TTL
  else Cache miss
    Fetcher->>External: HTTP request
    External-->>Fetcher: HTTP response (with/without Cache-Control/Expires)
    Fetcher->>Fetcher: parseCacheTTL(response headers, defaultTTL)
    Fetcher->>Cache: Store response with computed TTL
  end
  Fetcher-->>Server: Return data
  Server-->>Client: Respond
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nudged the host to cozy loopback ground,
An hour of TTL in config found,
Parsers tidy, bytes counted true,
Cache will sleep until headers coo,
Hop, fetch, and stash — a rabbit's bound.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title "Apply quality-of-life fixes" is vague and generic, using a non-descriptive term that doesn't convey the specific changes in the PR. Consider a more specific title that highlights the primary changes, such as "Make HTTP cache default TTL configurable and fix default bind address" or "Improve cache configuration with default TTL and tighten server binding."
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch qol-fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
internal/endorsements_test.go (1)

384-386: Use a non-default sentinel TTL in the no-header case.

This still exercises the new parameter with time.Hour, so an implementation that ignores defaultTTL and hardcodes 1h would also pass. Giving that case its own distinct TTL would actually lock down the new behavior.

Also applies to: 425-425

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/endorsements_test.go` around lines 384 - 386, The test case "no
headers returns default" currently uses time.Hour (the existing default) which
masks implementations that ignore the provided defaultTTL; change the sentinel
TTL in that case to a distinct non-default value (e.g., time.Minute or another
constant) so the test exercises the defaultTTL parameter, and make the same
change for the other occurrence referenced (the case at the later line). Update
the test cases in endorsements_test.go that set want: time.Hour for the
no-headers scenarios to use the new sentinel TTL and ensure any helper variables
(e.g., defaultTTL) or assertions still compare against that sentinel value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 335-339: The code converts humanize.ParseBytes result (n uint64)
to int64 without checking overflow; add a guard after ParseBytes in the same
function (where n, err := humanize.ParseBytes(s) is used) that returns an error
if n > uint64(math.MaxInt64) (use math.MaxInt64) so the function returns a clear
overflow error instead of silently truncating/producing a negative int64; keep
the existing error message style (e.g., fmt.Errorf) when returning the overflow
error.

In `@internal/cosign.go`:
- Around line 134-136: The code currently forces a default cache TTL whenever
ttl <= 0 which overwrites explicit non-cacheable or already-expired responses;
change the condition so the default is applied only when ttl == 0 (unspecified),
and preserve ttl when it's negative (explicit no-cache/no-store/expired). Update
the check that references ttl and s.cfg.HTTPCacheDefaultTTL in
internal/cosign.go so it sets ttl = s.cfg.HTTPCacheDefaultTTL only for ttl == 0
and does not override ttl < 0.

In `@internal/endorsements.go`:
- Around line 104-106: The current check in endorsements.go overrides a TTL of 0
(which parseCacheTTL uses to mean "no-cache"/"no-store"/expired), causing
responses that should not be cached to get the default TTL; locate the code that
calls parseCacheTTL (reference parseCacheTTL and the local variable ttl) and
change the conditional so you only apply s.cfg.HTTPCacheDefaultTTL when ttl is
negative (e.g. change "if ttl <= 0" to "if ttl < 0"), preserving ttl==0 values
returned intentionally by parseCacheTTL.

In `@internal/fetch.go`:
- Around line 123-124: The function parseCacheTTL currently returns defaultTTL
as-is, allowing configured defaults >24h to bypass the documented cap; update
parseCacheTTL to clamp defaultTTL to a maximum of 24 hours (time.Hour*24) at the
start (or before any early return) so any return path that yields defaultTTL
respects the 24h cap; ensure existing logic that parses the Expires header still
applies the same cap to parsed values as well.

---

Nitpick comments:
In `@internal/endorsements_test.go`:
- Around line 384-386: The test case "no headers returns default" currently uses
time.Hour (the existing default) which masks implementations that ignore the
provided defaultTTL; change the sentinel TTL in that case to a distinct
non-default value (e.g., time.Minute or another constant) so the test exercises
the defaultTTL parameter, and make the same change for the other occurrence
referenced (the case at the later line). Update the test cases in
endorsements_test.go that set want: time.Hour for the no-headers scenarios to
use the new sentinel TTL and ensure any helper variables (e.g., defaultTTL) or
assertions still compare against that sentinel value.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5c300660-fd7c-4163-83da-8f6bd23d7d4d

📥 Commits

Reviewing files that changed from the base of the PR and between 56c9322 and ad7ddbf.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (10)
  • CLAUDE.md
  • cmd/root.go
  • config/config.toml
  • go.mod
  • internal/config.go
  • internal/cosign.go
  • internal/endorsements.go
  • internal/endorsements_test.go
  • internal/fetch.go
  • internal/server.go

Comment thread internal/config.go
Comment thread internal/cosign.go Outdated
Comment thread internal/endorsements.go Outdated
Comment thread internal/fetch.go
sangwa and others added 3 commits March 25, 2026 18:35
A configured http.cache.default_ttl >24h would bypass the documented
24h cap because the default return path did not clamp the value.
Now parseCacheTTL clamps defaultTTL at entry so all return paths
(max-age, Expires, and default) respect the cap.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
humanize.ParseBytes returns uint64; values above math.MaxInt64 (e.g.
"9EiB") would silently wrap to negative on the int64 cast. Now returns
a clear error instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
parseCacheTTL returns 0 to signal "do not cache" (no-cache, no-store,
or expired Expires header). The ttl <= 0 guard in endorsements.go and
cosign.go was overriding this with the default TTL, causing responses
that should not be cached to be cached. Changed to ttl < 0 so only
truly invalid values get the fallback.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 316-325: Update the comment for parseDuration to accurately
reflect its behavior: state that it reads a viper string key and parses it as a
time.Duration and returns an error if the value is empty or cannot be parsed
(i.e., for empty strings and any unparseable duration strings). Reference the
parseDuration function and ensure the docstring no longer implies empty values
are allowed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 65248c58-2399-481a-ad3a-db093a1ac4ec

📥 Commits

Reviewing files that changed from the base of the PR and between ad7ddbf and 8b20cae.

📒 Files selected for processing (5)
  • internal/config.go
  • internal/cosign.go
  • internal/endorsements.go
  • internal/endorsements_test.go
  • internal/fetch.go
✅ Files skipped from review due to trivial changes (1)
  • internal/fetch.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/cosign.go
  • internal/endorsements.go
  • internal/endorsements_test.go

Comment thread internal/config.go
The comment said "non-empty and unparseable" implying empty strings
were accepted, but time.ParseDuration("") returns an error.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
internal/config.go (1)

316-344: Please add tests for these new startup-failure paths.

internal/config_test.go still doesn't cover invalid durations or byte-size overflow, even though this code now decides whether the server starts. A small table-driven test here would lock in the new fail-fast contract.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/config.go` around lines 316 - 344, Add table-driven tests in
internal/config_test.go to cover the new fail-fast paths in parseDuration and
parseByteSize: write cases that pass an empty/invalid duration string to
parseDuration (e.g., "" and "5xs") and assert it returns a non-nil error, and
cases for parseByteSize that pass empty string and an oversized value (a string
that parses to > math.MaxInt64) and assert errors; call the functions directly
(parseDuration, parseByteSize) and verify the error messages contain the
problematic input to lock in the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 118-136: Parsed durations from parseDuration (used to populate
endorsementTimeout, httpCacheDefaultTTL, revocationRefreshInterval,
rateLimitStallTimeout, etc.) can be negative and must be rejected; after each
parseDuration call check if the returned duration is < 0 and return an error
(e.g., fmt.Errorf("%s: negative duration %s", "<config.key>", d)) consistent
with existing parseByteSize error handling so invalid negative timeouts/TTLs
fail fast. Ensure you update all places that call parseDuration in this block
and include the specific config key name in the error message to aid debugging.

---

Nitpick comments:
In `@internal/config.go`:
- Around line 316-344: Add table-driven tests in internal/config_test.go to
cover the new fail-fast paths in parseDuration and parseByteSize: write cases
that pass an empty/invalid duration string to parseDuration (e.g., "" and "5xs")
and assert it returns a non-nil error, and cases for parseByteSize that pass
empty string and an oversized value (a string that parses to > math.MaxInt64)
and assert errors; call the functions directly (parseDuration, parseByteSize)
and verify the error messages contain the problematic input to lock in the
behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 187685cb-3fa3-4cdf-92a5-7a0002beaef8

📥 Commits

Reviewing files that changed from the base of the PR and between 8b20cae and c601b56.

📒 Files selected for processing (1)
  • internal/config.go

Comment thread internal/config.go
time.ParseDuration accepts negative values like "-5s" which are
nonsensical for timeouts and TTLs. Now returns a clear error with the
config key name. Added tests for parseDuration (empty, unparseable,
negative) and parseByteSize (empty, overflow) error paths.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 118-121: parseDuration currently accepts zero which breaks callers
that require positive durations (RevocationRefreshInterval used by
time.NewTicker, EndorsementClientTimeout used with
context.WithTimeout/http.Client.Timeout, and RateLimitStallTimeout used with
context.WithTimeout). Fix by making parseDuration return an error for zero
durations (treat zero as invalid) and update callers (e.g., where
endorsementTimeout is assigned in internal/config.go) to handle that error path;
alternatively, if you prefer per-callsite validation, add explicit checks that
parsed values for RevocationRefreshInterval, EndorsementClientTimeout, and
RateLimitStallTimeout are > 0 and return a clear error instead of allowing zero
to propagate.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f34da73-1561-497b-b113-4124e2d77367

📥 Commits

Reviewing files that changed from the base of the PR and between c601b56 and ae5e012.

📒 Files selected for processing (2)
  • internal/config.go
  • internal/config_test.go
✅ Files skipped from review due to trivial changes (1)
  • internal/config_test.go

Comment thread internal/config.go
endorsements.client.timeout (http.Client.Timeout=0 disables timeout),
revocation.refresh_interval (time.NewTicker(0) panics), and
ratelimit.stall_timeout (context.WithTimeout(ctx,0) expires instantly)
all require positive durations. http.cache.default_ttl is intentionally
left allowing zero (meaning "don't cache").

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/config.go (1)

122-146: Consider extracting a parsePositiveDuration helper to reduce repetition.

The pattern of calling parseDuration followed by a zero-check is repeated three times for fields that require positive values. A dedicated helper could consolidate this:

func parsePositiveDuration(key string) (time.Duration, error) {
    d, err := parseDuration(key)
    if err != nil {
        return 0, err
    }
    if d == 0 {
        return 0, fmt.Errorf("%s: must be positive", key)
    }
    return d, nil
}

This is a minor DRY improvement and entirely optional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/config.go` around lines 122 - 146, Extract a helper named
parsePositiveDuration that wraps parseDuration and enforces a non-zero result
(signature: parsePositiveDuration(key string) (time.Duration, error)); replace
the repeated parseDuration + zero-check blocks (used for keys
"http.cache.default_ttl", "revocation.refresh_interval", and
"ratelimit.stall_timeout") with calls to parsePositiveDuration, and update the
surrounding code to return the helper's error directly where those durations are
assigned (e.g., httpCacheDefaultTTL, revocationRefreshInterval,
rateLimitStallTimeout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/config.go`:
- Around line 122-146: Extract a helper named parsePositiveDuration that wraps
parseDuration and enforces a non-zero result (signature:
parsePositiveDuration(key string) (time.Duration, error)); replace the repeated
parseDuration + zero-check blocks (used for keys "http.cache.default_ttl",
"revocation.refresh_interval", and "ratelimit.stall_timeout") with calls to
parsePositiveDuration, and update the surrounding code to return the helper's
error directly where those durations are assigned (e.g., httpCacheDefaultTTL,
revocationRefreshInterval, rateLimitStallTimeout).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 20152d2d-0a75-4bf6-ba55-ee24d285077b

📥 Commits

Reviewing files that changed from the base of the PR and between ae5e012 and 11b3304.

📒 Files selected for processing (1)
  • internal/config.go

@sangwa sangwa merged commit cf94078 into main Mar 26, 2026
3 checks passed
@sangwa sangwa deleted the qol-fixes branch March 26, 2026 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant